Tutorials edits - Terminate on configuration error - Terminal Color#2368
Conversation
WalkthroughIntroduces terminal color support and detection, centralizes a JSON schema filename constant, changes bean activation to propagate failures, reorganizes text/XML utilities (new XMLTextUtil and util.text package), removes TerminateException from HTTP server handler signatures, updates startup scripts and logging configs, and applies numerous import/package adjustments. Changes
*Many files under core/src/main/java/... updated import paths to the new util.text package. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(1 hunks)distribution/tutorials/getting-started/00-First-API.yaml(1 hunks)distribution/tutorials/getting-started/10-Logging.yaml(1 hunks)distribution/tutorials/getting-started/40-Basic-Path-Routing.yaml(2 hunks)docs/ROADMAP.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2240
File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215
Timestamp: 2025-10-23T13:04:11.388Z
Learning: In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
🪛 markdownlint-cli2 (0.18.1)
docs/ROADMAP.md
19-19: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (8)
distribution/tutorials/getting-started/10-Logging.yaml (1)
12-12: LGTM!Adding the
./prefix improves clarity and follows shell script best practices for executing scripts in the current directory.distribution/tutorials/getting-started/40-Basic-Path-Routing.yaml (2)
32-33: Helpful informational comment.This comment appropriately warns users that the external API may occasionally fail, improving the tutorial experience.
41-42: Helpful informational comment.This comment appropriately warns users about potential response delays, setting proper expectations for the tutorial.
distribution/tutorials/getting-started/00-First-API.yaml (2)
8-29: LGTM!The formatting improvements (numbering style from
1)to1.and added spacing) enhance readability and consistency across the tutorial documentation.
33-33: Good practice to use HTTPS.Updating the troubleshooting example to use HTTPS is appropriate since the target API in the configuration uses HTTPS.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
144-147: Fail-fast behavior on configuration errors implemented correctly.The addition of
throw new RuntimeException(e);ensures that Membrane terminates immediately when a bean definition cannot be activated, preventing startup with invalid or partial configuration. This aligns with the PR objective "Terminate on configuration error" and follows the fail-fast principle for improved reliability.annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
171-171: LGTM!Correctly uses the newly defined constant, eliminating the hardcoded string and improving maintainability.
docs/ROADMAP.md (1)
18-22: LGTM!The roadmap additions are clear and well-formatted. Note: The static analysis warning about the bare URL on line 19 is a false positive—the URL is appropriately used in the roadmap context and doesn't require markdown link syntax.
predic8
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpEndpointListener.java (1)
95-95: Consider making ANSI color codes configurable or abstracting them.Hard-coded ANSI escape sequences will appear as raw text in log aggregators (Splunk, CloudWatch, ELK), break structured logging parsers, and clutter file-based logs. Consider:
- Adding a configuration flag to enable/disable colorized output
- Using a logging utility or constant to centralize color formatting
- Detecting terminal support before applying colors (e.g., check if
System.console()is available)</invoke_end -->
core/src/main/java/com/predic8/membrane/core/Router.java (1)
358-358: Consider making ANSI color codes configurable or abstracting them.Hard-coded ANSI escape sequences in log messages will appear as raw text in log aggregators, break structured logging, and clutter non-terminal outputs. The same formatting is applied in two places (lines 358 and 688), suggesting an opportunity to centralize this logic.
Consider:
- Adding a configuration option to enable/disable colorized console output
- Creating a utility method or constant for formatted messages
- Detecting if output is a TTY before applying colors
Also applies to: 688-688
core/src/main/java/com/predic8/membrane/core/proxies/ApiInfo.java (3)
37-37: Consider making ANSI color codes configurable or abstracting them.Hard-coded ANSI escape sequences will pollute logs in production environments where output is redirected to log aggregators or files. This affects observability and log parsing.
Consider centralizing color formatting logic with the same approach used in
HttpEndpointListenerandRouter, ideally with a configuration flag to control colorization.
45-45: Removed descriptive text reduces log clarity.The previous text " using OpenAPI specifications:" helped users understand what the subsequent indented list represented. Without this label, the output is less self-documenting, especially for operators unfamiliar with the system.
Consider retaining some descriptive text or ensuring the format is clearly documented.
71-71: APIProxy returns an empty prefix, creating format inconsistency.While other proxy types now have labeled prefixes (e.g., "Service:", "SOAP:"),
APIProxyreturns an empty string. This creates a formatting inconsistency in log output, though it may be an intentional design choice to treat API proxies as the default type.Consider whether APIProxy should have a prefix for consistency, or document this design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/Router.java(2 hunks)core/src/main/java/com/predic8/membrane/core/proxies/ApiInfo.java(2 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/HttpEndpointListener.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/main/java/com/predic8/membrane/core/proxies/ApiInfo.java (1)
74-74: LGTM: Consistent addition of colons improves log formatting.Adding colons to the proxy type labels (e.g., "Service:", "SOAP:", "Internal:", "Proxy:") provides consistent formatting and improves readability of the log output.
Also applies to: 77-77, 80-80, 82-82
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
distribution/tutorials/getting-started/00-First-API.yaml(1 hunks)distribution/tutorials/getting-started/10-Logging.yaml(1 hunks)distribution/tutorials/getting-started/40-Basic-Path-Routing.yaml(2 hunks)docs/ROADMAP.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/tutorials/getting-started/40-Basic-Path-Routing.yaml
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/ROADMAP.md
26-26: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
distribution/tutorials/getting-started/10-Logging.yaml (1)
13-13: Shell script path improved for explicit local execution.Adding
./prefix to the shell invocation is a best practice that ensures the local script is executed rather than relying on PATH resolution. This makes the tutorial more robust across different environments.distribution/tutorials/getting-started/00-First-API.yaml (3)
9-9: Improved tutorial structure with consistent formatting and next-step guidance.The numbering format update (
1)→1.)) provides visual consistency with standard enumeration. Adding step 4 with a reference to10-Logging.yamlcreates a clear tutorial progression path, which is especially helpful for users just starting.Also applies to: 19-19, 26-26, 29-29
13-13: Consistent shell script path improvement.This change aligns with the same update in
10-Logging.yamland ensures local script execution across both tutorials.
34-34: HTTPS upgrade improves security consistency.The troubleshooting section now correctly uses HTTPS (
https://api.predic8.de) in both the curl example and the API configuration. This ensures the tutorial is consistent with real-world API behavior and modern security practices.Also applies to: 39-39
docs/ROADMAP.md (1)
25-25: Roadmap additions are well-motivated.The new entries for startup log colorization and the bom distribution question align with the PR objectives and enhance roadmap clarity.
Also applies to: 27-29
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
…ed new `TerminalColors` and `XMLTextUtil` classes - Moved text-related utilities to `com.predic8.membrane.core.util.text` package. - Introduced `TerminalColors` class for ANSI terminal color handling. - Added `XMLTextUtil` class with utilities for XML formatting and validation. - Updated class references and imports across the codebase to reflect package restructuring. - Enhanced logging configuration to support color highlighting.
… warnings - Removed redundant methods no longer in use (`waitForAsynchronousInitialization`, `isAsynchronousInitialization`). - Improved type-checking with modern Java features (e.g., `instanceof` with variable binding). - Refined helper method signatures to reduce unnecessary parameters. - Suppressed unused warning in `TerminalColors`. - Updated exception types to match method requirements.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java (1)
177-221: Remove unnecessaryTerminateExceptionfrom method signatures
process()now declares onlythrows IOException, butprocessHttp1Request()andprocessSingleRequest()still includeTerminateExceptionin their throws clauses. No code path in either method actually throwsTerminateException—readAndParseRequest(),process(),determineConnectionContinuation(), andHttp2ServerHandler.handle()all omit it from their signatures. These declarations can be safely removed.
♻️ Duplicate comments (1)
docs/ROADMAP.md (1)
29-29: Format bare URL as markdown link (flagged in prior review).This bare URL violates markdown best practices (MD034). Wrap it in bracket-parenthesis syntax to create a proper link.
- Register JSON Schema for YAML at: https://www.schemastore.org/ + Register JSON Schema for YAML at: [schemastore.org](https://www.schemastore.org/)
🧹 Nitpick comments (4)
docs/ROADMAP.md (1)
21-23: Consider reformulating the @coderabbitai directive as a formal TODO or note.The instruction to "coderabbitai look through the code base for usages" embedded in line 23 appears to be an external directive rather than documentation. For a roadmap, consider rephrasing as a formal TODO or moving to an issue tracker so the intent is clearer to future readers.
Example reformulation:
- Central desciption of MEMBRANE_* environment variables - Like MEMBRANE_HOME... - @coderabbitai look through the code base for usages of these variables and suggest documentation + Central description of MEMBRANE_* environment variables - Like MEMBRANE_HOME... - TODO: Document all MEMBRANE_* environment variables and their usages throughout the codebaseannot/src/test/java/com/predic8/membrane/annot/util/GrammarMock.java (1)
26-26: Consider declaring field using theMapinterface type.For consistency with the "program to interfaces" principle, consider using
Map<String, Class<?>>as the field type. This is a minor style point for test utility code.- private HashMap<String, Class<?>> elements = new HashMap<>(); + private Map<String, Class<?>> elements = new HashMap<>();core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
120-160: ANSI detection logic is comprehensive and well-structured.The
detectAnsiSupport()method handles multiple environments appropriately:
- User override via
MEMBRANE_COLORSenvironment variable- CI environment detection
- IDE support (IntelliJ, VSCode)
- Terminal capability checks via
TERMvariable- Platform-specific logic for Windows
The implementation is thread-safe with the volatile flag and provides a clean API that returns empty strings when disabled.
One optional consideration: Line 159 returns
trueas a last-resort default, which is optimistic. In rare cases where none of the checks match, this could enable ANSI in unsupported environments. Consider whether returningfalsewould be safer, though the current approach favors modern terminal support.core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
15-16: yes()/no() helpers look correct; consider documenting accepted valuesThe new
yes(String)/no(String)methods are null‑safe and handle a good set of case‑insensitive tokens (on/off,yes/no,true/false,1/0,enable[d]/disable[d]). To make their contract clearer to callers, you could add brief Javadoc listing these values.@@ - public static boolean yes(String override) { + /** + * Returns {@code true} for common affirmative values (case-insensitive, + * surrounding whitespace ignored): {@code "on"}, {@code "yes"}, {@code "y"}, + * {@code "true"}, {@code "1"}, {@code "enable"}, {@code "enabled"}. + */ + public static boolean yes(String override) { @@ - public static boolean no(String override) { + /** + * Returns {@code true} for common negative values (case-insensitive, + * surrounding whitespace ignored): {@code "off"}, {@code "no"}, {@code "n"}, + * {@code "false"}, {@code "0"}, {@code "disable"}, {@code "disabled"}. + */ + public static boolean no(String override) {Also applies to: 65-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
annot/src/test/java/com/predic8/membrane/annot/util/GrammarMock.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/yaml/MethodSetterTest.java(1 hunks)core/src/main/java/com/predic8/membrane/core/Router.java(3 hunks)core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java(1 hunks)core/src/main/java/com/predic8/membrane/core/graphql/GraphQLProtectionInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/http/Message.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/acl/Resource.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/acl/matchers/GlobMatcher.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminPageBuilder.java(5 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/groovy/GroovyInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/javascript/JavascriptInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/log/access/AccessLogInterceptorService.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/javascript/RhinoJavascriptLanguageSupport.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELCookie.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELHeader.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java(1 hunks)core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/proxies/ApiInfo.java(3 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java(1 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/HttpEndpointListener.java(3 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/json/JsonToXml.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/http/Http11Test.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/lang/spel/spelable/SPeLablePropertyAwareTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/StringUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/json/JsonToXmlTest.java(1 hunks)core/src/test/resources/log4j2.xml(1 hunks)distribution/conf/log4j2.xml(1 hunks)distribution/router/conf/log4j2.xml(1 hunks)distribution/src/main/java/com/predic8/membrane/core/FilterExamples.java(1 hunks)docs/ROADMAP.md(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTInterceptor.java
- core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java
- core/src/test/java/com/predic8/membrane/core/util/json/JsonToXmlTest.java
- core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELCookie.java
- core/src/main/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptor.java
- core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java
- core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java
- core/src/main/java/com/predic8/membrane/core/lang/javascript/RhinoJavascriptLanguageSupport.java
- distribution/src/main/java/com/predic8/membrane/core/FilterExamples.java
- core/src/main/java/com/predic8/membrane/core/interceptor/groovy/GroovyInterceptor.java
- core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/Router.java
- core/src/main/java/com/predic8/membrane/core/proxies/ApiInfo.java
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.931Z
Learning: The mediaType method in the Request class throws jakarta.mail.internet.ParseException, not java.text.ParseException, making the jakarta.mail.internet.ParseException import necessary and correct in JsonSchemaTestSuiteTests.java.
Applied to files:
core/src/test/java/com/predic8/membrane/core/http/Http11Test.java
📚 Learning: 2025-11-15T18:38:23.728Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.728Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.java
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.931Z
Learning: The mediaType method in the Message class (which Request extends) throws jakarta.mail.internet.ParseException because it creates a ContentType object for parsing MIME media types. The jakarta.mail.internet.ParseException import is correct and necessary in test files that call mediaType().
Applied to files:
core/src/main/java/com/predic8/membrane/core/http/Message.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws Exception, not XMLStreamException specifically, so catching IOException in TextUtil.formatXML is appropriate for handling IO errors during XML parsing.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.javacore/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws IOException specifically, not Exception. It internally wraps any XMLStreamException or other parsing exceptions in an IOException. Therefore, catching IOException in TextUtil.formatXML is the correct approach for handling XML parsing errors from InputStream.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.javacore/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) and XMLBeautifier.parse(Reader) both declare "throws Exception" in their signatures, not XMLStreamException specifically. Catching IOException is appropriate for handling IO errors during XML parsing with XMLBeautifier.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.javacore/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java
🧬 Code graph analysis (23)
core/src/test/java/com/predic8/membrane/core/http/Http11Test.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java (1)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
XMLTextUtil(27-96)
core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
XMLTextUtil(27-96)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(19-86)
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
XMLTextUtil(27-96)
core/src/test/java/com/predic8/membrane/core/util/StringUtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(19-86)
core/src/main/java/com/predic8/membrane/core/interceptor/javascript/JavascriptInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELHeader.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/interceptor/log/access/AccessLogInterceptorService.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/interceptor/acl/Resource.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/test/java/com/predic8/membrane/core/lang/spel/spelable/SPeLablePropertyAwareTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminPageBuilder.java (1)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
XMLTextUtil(27-96)
core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/http/Message.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(19-86)
core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(19-86)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(19-86)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(21-52)
core/src/main/java/com/predic8/membrane/core/interceptor/acl/matchers/GlobMatcher.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-270)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
XMLInputFactoryFactory(24-60)
🪛 LanguageTool
docs/ROADMAP.md
[grammar] ~21-~21: Ensure spelling is correct
Context: ...ystem.console() is available) - Central desciption of MEMBRANE_* environment variables -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/ROADMAP.md
29-29: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (35)
core/src/main/java/com/predic8/membrane/core/util/json/JsonToXml.java (1)
1-14: LGTM!The Apache 2.0 license header is correctly formatted and aligns with the PR's objective of adding license headers across the codebase.
core/src/test/java/com/predic8/membrane/core/lang/spel/spelable/SPeLablePropertyAwareTest.java (1)
21-21: LGTM! Clean import path update.The static import has been correctly updated to reflect the relocation of
TextUtilto theutil.textpackage.core/src/main/java/com/predic8/membrane/core/interceptor/acl/Resource.java (1)
31-31: LGTM! Clean refactoring.The import path update correctly reflects the new package structure for TextUtil. The usage at line 69 remains valid and unchanged.
annot/src/test/java/com/predic8/membrane/annot/util/GrammarMock.java (2)
28-46: LGTM!The interface method implementations are appropriate for a test mock. Returning
null/empty values for unneeded methods and delegating to the internal map forgetElementis clean and functional.
48-51: LGTM!The fluent builder method provides good ergonomics for test setup, enabling clean method chaining when configuring the mock.
annot/src/test/java/com/predic8/membrane/annot/yaml/MethodSetterTest.java (1)
1-13: Apache 2.0 license header is correct and well‑placedThe header text matches the standard Apache 2.0 license wording and is correctly positioned above the package declaration; no issues from a legal or technical standpoint.
core/src/main/java/com/predic8/membrane/core/interceptor/javascript/JavascriptInterceptor.java (1)
23-23: LGTM!The import path update aligns with the TextUtil package reorganization into
util.text.core/src/main/java/com/predic8/membrane/core/http/Message.java (1)
28-28: LGTM!The static import update correctly reflects the TextUtil package relocation.
core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.java (1)
26-26: LGTM!The import path change is consistent with the TextUtil package reorganization.
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
31-32: Private constructor correctly prevents instantiation.This is the appropriate pattern for a utility class with only static methods.
core/src/main/java/com/predic8/membrane/core/transport/http/HttpEndpointListener.java (1)
20-20: LGTM! Colorized listening address improves startup visibility.The use of
BRIGHT_MAGENTA()for the listening address makes it stand out in logs, which is helpful for quickly identifying where the service is running. The implementation correctly wraps the address with color codes and resets afterward, and will gracefully degrade to plain text when colors are disabled.Also applies to: 33-33, 97-97
core/src/test/resources/log4j2.xml (1)
2-2: Test logging configuration enables ANSI colors.While
disableAnsi="false"is consistent with the other logging configurations, note thatTerminalColors.detectAnsiSupport()specifically checks for CI environments and returns false. However, Log4j2 will still emit ANSI codes since it's configured to do so at the framework level. This typically works fine in modern CI systems, but if test logs appear garbled in CI output, this would be the configuration to adjust.Also applies to: 5-7
core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
22-24: LGTM! Windows detection simplified.The change from checking
"windows"to"win"is appropriate and makes the detection more robust across different Windows versions (Windows 10, Windows 11, Windows Server, etc.).core/src/test/java/com/predic8/membrane/core/http/Http11Test.java (1)
26-26: LGTM! Import path updated for TextUtil package reorganization.The static import change aligns with the broader refactor moving TextUtil to the
util.textsubpackage.core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java (1)
22-22: LGTM! Import path updated for TextUtil package reorganization.The static import change aligns with the relocation of TextUtil to the
util.textsubpackage.core/src/main/java/com/predic8/membrane/core/interceptor/acl/matchers/GlobMatcher.java (1)
18-18: LGTM! Import path updated for TextUtil package reorganization.The static import change for
globToRegExpaligns with the relocation of TextUtil to theutil.textsubpackage.core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java (1)
32-32: LGTM! Import updated to use XMLTextUtil for XML formatting.The change from
TextUtil.*toXMLTextUtil.*appropriately separates XML-specific formatting utilities (likeformatXMLused on line 184) into a dedicated utility class.core/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.java (1)
22-22: LGTM! Import path updated for util package reorganization.The import change aligns with the relocation of text utilities to the
util.textsubpackage. TextUtil usage on line 316 remains unchanged.core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELHeader.java (1)
20-20: LGTM! Import path updated for TextUtil package reorganization.The static import change for
camelToKebabaligns with the relocation of TextUtil to theutil.textsubpackage. Usage on line 35 remains unchanged.core/src/main/java/com/predic8/membrane/core/graphql/GraphQLProtectionInterceptor.java (1)
23-23: LGTM! Import path updated for util package reorganization.The import change aligns with the relocation of text utilities to the
util.textsubpackage. TextUtil usage on line 190 remains unchanged.core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (1)
34-34: Import path update to new StringUtil package looks correct
truncateAfteris now sourced fromcom.predic8.membrane.core.util.text.StringUtil, which matches the refactored utility location; behavior stays the same.core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (1)
41-41: maskNonPrintableCharacters import aligned with util.text.StringUtilThe updated static import correctly targets
com.predic8.membrane.core.util.text.StringUtil.maskNonPrintableCharacters, keepingassignOpenAPINamebehavior unchanged.core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (1)
34-34: addLineNumbers now consistently sourced from text.StringUtilThe static import switch to
com.predic8.membrane.core.util.text.StringUtil.addLineNumbersmatches the refactored utility class and keeps template error diagnostics intact.core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (1)
25-25: TextUtil import updated to util.text package
getLongDescription()continues to useTextUtil.toEnglishListwith the class now correctly imported fromcom.predic8.membrane.core.util.text.TextUtil.core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java (1)
39-39: Switch to XMLTextUtil.formatXML matches XML beautification usageUsing
com.predic8.membrane.core.util.xml.XMLTextUtil.formatXMLforgetBeautifiedBodyaligns with the XML‑focused utility class and matches the(InputStream, boolean)signature.core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java (1)
26-26: Tests now use XMLTextUtil.formatXML consistentlyThe static import and
getContentimplementation correctly targetXMLTextUtil.formatXML(Reader), keeping the test behavior while aligning with the new XML utility class.Also applies to: 157-159
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java (1)
36-36: linkURL import moved to util.text.TextUtil
getLongDescription()now pullslinkURLfromcom.predic8.membrane.core.util.text.TextUtil, matching the refactored text utilities without altering output.core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java (1)
32-32: Connection now imports text utilities from util.text.TextUtilThe wildcard static import has been repointed to
com.predic8.membrane.core.util.text.TextUtil.*, keepingisNullOrEmptyand any future text helpers available without changing connection logic.core/src/main/java/com/predic8/membrane/core/interceptor/log/access/AccessLogInterceptorService.java (1)
30-32: Static import now correctly targets moved TextUtilSwitching the static import to
com.predic8.membrane.core.util.text.TextUtil.*keepsescapeQuotesresolution consistent with the TextUtil move. No functional changes in this class.core/src/test/java/com/predic8/membrane/core/util/StringUtilTest.java (1)
19-20: Test static import aligned with new StringUtil packageImporting from
com.predic8.membrane.core.util.text.StringUtil.*matches the class move; the existing tests still exercise the same APIs.core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminPageBuilder.java (2)
38-39: XML snippet validation correctly moved to XMLTextUtilUsing
XMLTextUtil.isValidXMLSnippetforshortDescription/longDescriptionkeeps the previous behavior of only inlining valid XML and HTML‑escaping everything else, while separating XML logic from generic TextUtil. Looks consistent with the new utility split.Also applies to: 621-627
637-642: Refactored help/expand icon helpers keep IDs consistent with JS
createHelpIcon(Interceptor)andcreateExpandIcon(String)integrate cleanly withcreateShowIconsScript: thei*,e*, andc*IDs still match the jQuery selectors, and nullhelpIdis handled gracefully. No functional regressions apparent.Also applies to: 703-721
core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java (1)
38-40: StringUtil import move preserves masking/truncation behaviorUsing
com.predic8.membrane.core.util.text.StringUtil.maskNonPrintableCharactersandtruncateAfterkeepsgetLineMaskedAndTruncatedidentical in behavior after the package move.Also applies to: 159-161
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
15-16: TextUtil package move cleanly separates text vs. XML concernsMoving
TextUtiltocom.predic8.membrane.core.util.textand dropping XML‑specific helpers (now inXMLTextUtil) keeps existing text APIs intact while clarifying responsibilities between text and XML utilities.core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
72-95: XML snippet validation matches previous behavior and uses hardened XMLInputFactory
isValidXMLSnippetwraps the snippet in a synthetic root element and parses it viaXMLInputFactoryFactory.inputFactory(), then checks for anEndDocumentevent. This preserves the earlier semantics for “balanced, well‑formed snippet” while benefiting from the existing XMLInputFactory hardening (no external entities, DTD disabled, etc.). The resource is closed in a finally block, and failures only result in afalsereturn.
…e consistency - Changed fallback return value in `TerminalColors` to `false` for safer behavior. - Updated exception type in `XMLTextUtil` to match method operations. - Refined imports in `GrammarMock` for conciseness. - Removed unused exceptions in `HttpServerHandler` to simplify method signatures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
22-24: Avoid class‑level@SuppressWarnings("unused")where possiblePutting
@SuppressWarnings("unused")on the whole class will also hide genuinely unused methods if this utility grows. Consider either:
- Dropping the suppression entirely and letting the compiler/IDE show unused members, or
- Narrowing the suppression to specific methods that are intentionally kept for future use.
120-160: Heuristics indetectAnsiSupport()could be slightly tightened and hardenedThe overall flow (explicit
MEMBRANE_COLORSoverride → CI guard → IDE/terminal heuristics → OS fallback) is reasonable, but you might want to consider:
- Wrapping the
System.getenv/System.getPropertycalls in a small try/catch forSecurityExceptionand falling back tofalse, to avoid class‑init failures in restricted environments.- Reconsidering the very broad
TERM_PROGRAM != nullcheck (Lines 142‑143). It currently enables colors for any unknown program that sets this variable; restricting this to known good values or OS combinations would reduce the chance of mis‑detecting dumb terminals.These are minor robustness tweaks; behavior as written is still acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
annot/src/test/java/com/predic8/membrane/annot/util/GrammarMock.java(1 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java(4 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws Exception, not XMLStreamException specifically, so catching IOException in TextUtil.formatXML is appropriate for handling IO errors during XML parsing.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws IOException specifically, not Exception. It internally wraps any XMLStreamException or other parsing exceptions in an IOException. Therefore, catching IOException in TextUtil.formatXML is the correct approach for handling XML parsing errors from InputStream.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) and XMLBeautifier.parse(Reader) both declare "throws Exception" in their signatures, not XMLStreamException specifically. Catching IOException is appropriate for handling IO errors during XML parsing with XMLBeautifier.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java
🧬 Code graph analysis (4)
annot/src/test/java/com/predic8/membrane/annot/util/GrammarMock.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
Grammar(30-207)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
XMLInputFactoryFactory(24-60)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(19-86)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(21-52)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(19-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
annot/src/test/java/com/predic8/membrane/annot/util/GrammarMock.java (2)
1-14: License header looks correct and consistentApache 2.0 header and year update to 2025 look fine and in line with the rest of the project.
19-25: Good switch toMapinterface andfinalfieldUsing
Map<String, Class<?>>with afinalreference andHashMapas the implementation is a solid, idiomatic improvement for this test helper; no behavioral change and clearer intent.core/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.java (1)
1-97: Previous review feedback has been addressed; implementation looks solid.The Javadoc for the
asHTMLparameter now correctly describes HTML vs. plain XML formatting, and bothformatXMLoverloads properly catch and logIOExceptionas recommended. The implementation demonstrates good practices:
isValidXMLSnippetuses secure XML parsing viaXMLInputFactoryFactory(which disables external entities and DTDs)- Proper resource cleanup with exception suppression in the finally block
- Appropriate logging levels (info for I/O errors, debug for validation failures)
- The snippet validation wraps content in a root element to handle fragments correctly
core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java (1)
38-39: LGTM! Clean refactoring of imports and control flow.The import path updates from
com.predic8.membrane.core.util.StringUtiltocom.predic8.membrane.core.util.text.StringUtilcorrectly reflect the package restructuring. The removal ofTerminateExceptionfrom method signatures (lines 177, 208, 267) represents a good refactoring from exception-based control flow to return-based control flow usingRequestProcessingResult, which is more appropriate for non-exceptional termination conditions.Also applies to: 177-177, 208-208, 267-267
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
46-118: Color API design and enable/disable behavior look solidThe separation between raw codes (
*_) and the public methods that honor theenabledflag is clean, and the wrappers (error/warn/info/success,bright*,bold/underline) all correctly degrade to plain strings when colors are disabled. This should make it easy to adopt colors incrementally without risking corrupted log output.
- Corrected OS detection for Windows by using full "windows" string match. - Added 7.1.0 section to the roadmap, noting JSON Schema registration.
# Conflicts: # core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELCookie.java # core/src/main/java/com/predic8/membrane/core/lang/spel/spelable/SpELHeader.java # core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java # core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java # core/src/test/java/com/predic8/membrane/core/lang/spel/spelable/SPeLablePropertyAwareTest.java # core/src/test/java/com/predic8/membrane/core/util/StringUtilTest.java
…`TextUtil` usage, reorganize utility imports, and fix minor inconsistencies
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/ROADMAP.md (2)
17-17: Format bare URL as a markdown link (duplicate of previous review).The URL violates markdown best practices (MD034). Wrap it in bracket-parenthesis syntax to create a proper link.
-- Register JSON Schema for YAML at: https://www.schemastore.org +- Register JSON Schema for YAML at: [schemastore.org](https://www.schemastore.org/)
24-24: Fix spelling error: "desciption" → "description" (duplicate of previous review).Typo in roadmap item description persists from previous review.
-- Central desciption of MEMBRANE_* environment variables +- Central description of MEMBRANE_* environment variables
🧹 Nitpick comments (2)
core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java (1)
22-24: Remove redundant specific static imports.Line 22's wildcard import
static com.predic8.membrane.core.util.text.TextUtil.*already imports all static methods fromTextUtil, making the specific imports on lines 23-24 redundant.Apply this diff to remove the redundant imports:
import static com.predic8.membrane.core.util.text.TextUtil.*; -import static com.predic8.membrane.core.util.text.TextUtil.getLineFromMultilineString; -import static com.predic8.membrane.core.util.text.TextUtil.removeFinalChar; import static java.nio.charset.StandardCharsets.*;core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
18-18: Remove unused import.The regular import of
StringUtilon line 18 is not used anywhere in the code. Only the static imports on lines 20-21 are actually used.Apply this diff:
package com.predic8.membrane.core.util.text; import com.predic8.membrane.core.util.*; -import com.predic8.membrane.core.util.StringUtil; import static com.predic8.membrane.core.util.StringUtil.no;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/main/java/com/predic8/membrane/core/graphql/GraphQLProtectionInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/javascript/RhinoJavascriptLanguageSupport.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java(0 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java(3 hunks)core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java(4 hunks)docs/ROADMAP.md(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java
- core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws Exception, not XMLStreamException specifically, so catching IOException in TextUtil.formatXML is appropriate for handling IO errors during XML parsing.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws IOException specifically, not Exception. It internally wraps any XMLStreamException or other parsing exceptions in an IOException. Therefore, catching IOException in TextUtil.formatXML is the correct approach for handling XML parsing errors from InputStream.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java
🧬 Code graph analysis (5)
core/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-332)
core/src/main/java/com/predic8/membrane/core/lang/javascript/RhinoJavascriptLanguageSupport.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-332)
core/src/main/java/com/predic8/membrane/core/graphql/GraphQLProtectionInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-332)
core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-332)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(21-91)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(21-52)
🪛 LanguageTool
docs/ROADMAP.md
[grammar] ~24-~24: Ensure spelling is correct
Context: ...ystem.console() is available) - Central desciption of MEMBRANE_* environment variables -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/ROADMAP.md
17-17: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (10)
core/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.java (2)
23-23: Static text-util imports are consistent with the new package usageUsing
com.predic8.membrane.core.util.text.*plus a static import ofTextUtil.*matches the refactor pattern in this PR and does not change behavior.Also applies to: 40-40
318-318: getShortDescription now uses static import; behavior remains the sameSwitching from
TextUtil.linkURL(docBase)tolinkURL(docBase)via static import is behaviorally identical and continues to HTML-escape / linkifydocBaseas before.core/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.java (1)
177-177: The refactoring from exception-based to return-value-based termination signaling is safe and complete.The removal of
TerminateExceptionfrom the throws clauses ofprocessSingleRequest,processHttp1Request, andprocessis correct. No code in the codebase catches or expectsTerminateExceptionfrom these methods. The shift to signaling termination viaRequestProcessingResultreturn values is cleaner and more explicit than exception-based control flow.core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
15-15: LGTM! Package relocation.The package move to
com.predic8.membrane.core.util.textappropriately organizes text-related utilities into a dedicated subpackage.core/src/main/java/com/predic8/membrane/core/lang/javascript/RhinoJavascriptLanguageSupport.java (1)
18-18: LGTM! Import updates align with package refactoring.The addition of the text package import and static import of
TextUtil.*correctly reflects the package relocation, and the simplified method call on line 55 appropriately leverages the static import.Also applies to: 26-27, 55-55
core/src/main/java/com/predic8/membrane/core/graphql/GraphQLProtectionInterceptor.java (1)
24-24: LGTM! Consistent with package refactoring.The import updates and simplified method call align with the TextUtil package relocation to
com.predic8.membrane.core.util.text.Also applies to: 33-33, 192-192
core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java (1)
81-97: LGTM! Test updates consistent with static imports.The method calls correctly leverage the static imports, and the test method rename maintains test coverage.
Also applies to: 231-237
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (3)
27-48: LGTM! Well-designed utility class.The class structure is appropriate:
finalclass with private constructor prevents instantiation and extensionvolatileflag ensures thread-safe visibility- Static initializer with auto-detection provides good defaults
- Public control methods offer flexibility
50-98: LGTM! Clean ANSI code implementation.The approach of private constants with public getters that respect the
enabledflag is elegant and allows colors to be cleanly disabled without null checks or conditionals at call sites.
124-164: LGTM! Comprehensive environment detection.The
detectAnsiSupport()method provides thorough environment detection with sensible defaults:
- User override via
MEMBRANE_COLORSprovides control- CI environment detection prevents color output in build logs
- IDE detection (IntelliJ, VSCode) enables colors appropriately
- Terminal capability checks (
TERM,TERM_PROGRAM)- Windows-specific handling for modern terminals
The logic progression from explicit override → environment detection → conservative default is well-structured.
…consolidating wildcard usage
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
19-22: FixStringUtilstatic import target and remove commented-out imports.
detectAnsiSupport()usesyes/no, which (percore/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java) live in thecom.predic8.membrane.core.util.textpackage, while the current static import points tocom.predic8.membrane.core.util.StringUtil.*. This is likely a compile error unless a secondStringUtilexists, and the commented-out imports are stale.Consider tightening the import and dropping the dead code:
-import static com.predic8.membrane.core.util.StringUtil.*; - -//import static com.predic8.membrane.core.util.StringUtil.no; -//import static com.predic8.membrane.core.util.text.StringUtil.yes; +import static com.predic8.membrane.core.util.text.StringUtil.no; +import static com.predic8.membrane.core.util.text.StringUtil.yes;You can confirm the actual
StringUtillocation(s) with:#!/bin/bash fd StringUtil.java rg -n "class StringUtil" -S
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java (1)
228-234: Consider renaming test method to follow Java naming conventions.The test method name
remove_Final_Charuses snake_case with mixed capitalization, which is inconsistent with Java naming conventions and other test methods in this class (e.g.,getLineFromMultilineStringTest,escapeQuoteSimple).Consider renaming to camelCase:
- void remove_Final_Char() { + void removeFinalChar() {or
- void remove_Final_Char() { + void testRemoveFinalChar() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T17:55:06.089Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:178-183
Timestamp: 2025-08-24T17:55:06.089Z
Learning: The capitalizeFirstCharacter method in TextUtil.java incorrectly uppercases the entire string instead of just the first character due to calling .toUpperCase() on the concatenated result. The method should only capitalize the first Unicode code point and leave the rest unchanged.
Applied to files:
core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(21-91)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(21-52)
core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
TextUtil(33-332)
🔇 Additional comments (2)
core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java (2)
21-21: LGTM: Static import updated correctly.The static import path has been correctly updated to reflect the relocation of
TextUtilto theutil.textpackage.
78-92: LGTM: Method calls simplified correctly.The method calls now leverage the static import, making the code more concise while maintaining the same functionality.
…ity methods - Added colorized logging for different log levels using `TerminalColors`. - Refined `OSUtil` with new constants, IntelliJ/VSCode detection, and JVM argument helper. - Simplified ANSI support detection logic in `TerminalColors`.
- Removed redundant imports and simplified method calls in `TerminalColors` and `OSUtil`. - Unified OS detection patterns for macOS, Windows, and Linux/Unix systems. - Improved readability and consistency in terminal color support logic.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
20-21: Remove commented-out imports.These commented-out import statements should be removed from the codebase.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java (1)
141-143: LGTM: Colored log output for better terminal visibility.The colored output enhances readability in terminal environments. The
TerminalColors.detectAnsiSupport()logic appropriately disables colors in CI and non-terminal contexts.Consider documenting in the class Javadoc that terminal colors are applied when supported, and that they can be controlled via the
MEMBRANE_COLORSenvironment variable if users need to override the auto-detection behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(23-84)core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(21-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (3)
26-30: LGTM: Thread-safe initialization.The use of
volatileensures visibility across threads for the enable/disable toggle, and the static initializer safely detects ANSI support at class load time.
221-253: LGTM: Comprehensive ANSI support detection.The detection logic correctly prioritizes user override, appropriately disables colors in CI environments, and provides sensible platform-specific checks with a conservative default.
74-219: LGTM: Color getters and wrapper methods.All color/style getters correctly respect the
enabledflag, and wrapper methods properly apply color codes with reset sequences.core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java (1)
23-23: LGTM: Necessary imports added.The imports correctly reference the new
TerminalColorsutility and text utilities package.Also applies to: 32-32
core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
59-62: LGTM!The
getJvmArgs()implementation is correct. The@NotNullannotation is appropriate sinceString.join()never returnsnull.
- Added system property `MEMBRANE_COLORS` to enable/disable terminal color logging explicitly. - Fixed null-safe access in OS detection methods. - Corrected method naming inconsistencies in `OSUtil` and `TerminalColors`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
20-21: Remove commented‑out imports.These commented static imports are obsolete and add noise, especially with the wildcard static import already in place. Please delete them.
-//import static com.predic8.membrane.core.util.StringUtil.no; -//import static com.predic8.membrane.core.util.text.StringUtil.yes;
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
221-253: Environment‑based ANSI detection is reasonable; consider reusing existing constants.The detection flow (explicit
MEMBRANE_COLORSoverride → CI off → IDE/terminal checks → OS/TERM heuristics) is sensible and conservative. One small improvement: to avoid string duplication, you could reuse the existingTERM_PROGRAM_ENVconstant fromOSUtilinstead of hardcoding"TERM_PROGRAM"here.For example:
- if (isMac()) { - // macOS terminal / iTerm2 / xterm / gnome-terminal - return System.getenv("TERM_PROGRAM") != null; - } + if (isMac()) { + // macOS terminal / iTerm2 / xterm / gnome-terminal + return System.getenv(TERM_PROGRAM_ENV) != null; + } @@ - // Windows Terminal - if (System.getenv("WT_SESSION") != null) return true; - return System.getenv("TERM_PROGRAM") != null; // classic cmd/powershell + // Windows Terminal + if (System.getenv("WT_SESSION") != null) return true; + return System.getenv(TERM_PROGRAM_ENV) != null; // classic cmd/powershellcore/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
25-38: OS detection improvements look good; consider making the Linux check more explicit.The switch to
OS_NAME_PROPERTYwith a null-safe default is solid and avoids NPEs. ForisLinux(), consider checking for"linux"instead of"inx"to make the intent clearer and avoid surprising matches in any future exoticos.namevalues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(23-83)core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(21-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (6)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (3)
26-45: Enabled flag initialization and toggling look correct.Using a volatile
enabledflag initialized viadetectAnsiSupport()and controlled throughenable()/disable()/isEnabled()is thread‑safe and straightforward. No issues here.
47-149: Color/style constants and getters behave as expected.ANSI codes are correct, and returning
""when disabled ensures callers get plain text without needing extra guards. This pattern is consistent across all colors and styles.
150-219: Semantic aliases and wrappers are consistent and degrade gracefully.
ERROR/WARN/INFO/SUCCESSand the bright/bold/underline wrappers correctly compose color + text + reset, and when colors are disabled they reduce to plain text. This is a clean and predictable API.core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (3)
59-62: Jvm-args helper and @NotNull contract look appropriate.Centralizing the JVM argument retrieval in
getJvmArgs()with an explicit@NotNullreturn type is clear and straightforward; no issues from my side.
64-78: IntelliJ environment detection is coherent and no longer conflates VSCode.Combining JVM-argument checks with IntelliJ-specific system properties and terminal env vars gives a reasonable signal for “runs in IntelliJ”, and keeping VSCode out of this method aligns with the naming.
80-82: VSCode terminal detection is simple and null-safe.Using
"vscode".equalsIgnoreCase(System.getenv(TERM_PROGRAM_ENV))correctly handles missing env vars without throwing, and matches the intended TERM_PROGRAM contract; this looks fine as-is.
… detection - Added new constants for terminal environment variables in `OSUtil`. - Introduced `isDumbTerminal` method for enhanced Linux/Unix support. - Refactored `detectAnsiSupport` in `TerminalColors` for better readability and logic reuse.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
21-22: Remove commented-out imports.These commented lines serve no purpose and should be removed to keep the codebase clean.
235-235: Fix the typo in the method name.The method
runsInVSCodeTerminal()has a typo with a capital 'E' in the middle of "Terminal". This should be corrected in both the method definition inOSUtil.javaand this call site.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
222-253: Reconsider the isDumbTerminal() logic—the method name is inverted from its behavior.The
isDumbTerminal()method (OSUtil.java, lines 88-94) returnstruewhen the TERM environment variable is set AND not equal to "dumb", meaning it returnstruefor capable terminals, not dumb ones. This contradicts the method name. At line 249, the code correctly enables ANSI support by checking this method, so the logic itself is not inverted. However, the method name is highly misleading and should be renamed to something likeisSmartTerminal()orsupportsAnsi()to match its actual behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/Constants.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (11)
core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (5)
24-31: LGTM! Good use of constants for magic strings.Extracting environment variable names and property names into public constants improves maintainability and enables reuse across the codebase.
32-42: LGTM! Null-safe OS detection.The addition of default empty strings to
System.getProperty()calls properly addresses the null-safety concern. Using"windows"instead of"win"correctly avoids false positives with "Darwin".
63-66: LGTM!The method correctly joins JVM arguments and the
@NotNullannotation is appropriate sinceString.join()will return an empty string for an empty list.
68-82: LGTM! Clean separation of IntelliJ detection logic.The VSCode check has been correctly removed, and the method now accurately detects only IntelliJ environments. The comments clearly separate Run/Debug/Test detection from Terminal detection.
84-86: LGTM! Method is null-safe.The typo has been fixed. Note that this implementation is actually null-safe because
"vscode".equalsIgnoreCase(null)returnsfalserather than throwing NPE—theequalsIgnoreCasemethod handles null arguments gracefully when called on a non-null String.core/src/main/java/com/predic8/membrane/core/Constants.java (1)
27-27: LGTM!The new constant follows the existing naming convention and provides a centralized reference for the environment variable used to control terminal color output.
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (5)
24-46: LGTM!The class structure follows best practices:
finalclass prevents extension- Private constructor prevents instantiation
volatileensures thread-safe reads/writes of theenabledflag- Static initialization is safe during class loading
49-72: LGTM!The ANSI escape codes are correct and follow the standard format. The naming convention with trailing underscores clearly distinguishes the private constants from the public getter methods.
75-149: LGTM!The getter methods follow a consistent pattern, checking the
enabledflag and returning either the ANSI code or an empty string. This design allows colors to be toggled without changing call sites.
152-166: LGTM!Semantic aliases improve code readability and make it easier to maintain a consistent color scheme across the application.
169-220: LGTM!The wrapper methods provide a convenient API for colorizing text without requiring manual reset codes. The implementation is consistent and correct.
… consistency - Updated `MEMBRANE_COLORS` to `MEMBRANE_TERM_COLORS` and added new system property `membrane.term.colors`. - Adjusted references in `LogInterceptor`, `TerminalColors`, and `Constants`. - Improved logic in `TerminalColors` for explicit fallback handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
21-21: Consider removing@SuppressWarnings("unused")and tighteningdetectAnsiSupport()visibilityWith
TerminalColorsnow used from other classes (e.g., Router, HttpEndpointListener, LogInterceptor), the class-level@SuppressWarnings("unused")is likely no longer necessary and may hide genuinely unused helpers over time. Dropping it would let the compiler/IDE surface real dead code if it appears.Also, unless you intend
detectAnsiSupport()to be part of the public API, you could narrow its visibility (e.g.,private staticor package‑private) since it’s already invoked from the static initializer and not obviously needed by callers. That keeps the public surface focused onenable/disable/isEnabledand the color/style accessors.Also applies to: 219-219
219-254: Clarify theisDumbTerminal()‑based Linux detection to avoid confusionOn the “Linux” path you enable colors via:
if (isDumbTerminal()) return true;But per
OSUtil.isDumbTerminal()(incore/src/main/java/com/predic8/membrane/core/util/OSUtil.java), the method actually returnstruewhenTERMis not"dumb"and is set, which is correct for enabling ANSI but opposite to what the name suggests.The behavior is fine, but the naming/usage can be surprising. Consider either:
- Renaming
OSUtil.isDumbTerminal()to something likehasColorCapableTerm()(and inverting its logic), or- Adding a short comment here that explicitly notes that
isDumbTerminal()returnstruefor non‑"dumb"terminals, so future readers don’t misinterpret this condition.This is mainly a readability/maintenance improvement; functionally the detection logic is sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/Constants.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java
- core/src/main/java/com/predic8/membrane/core/Constants.java
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (3)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-99)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(23-95)core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(21-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Automated tests
🔇 Additional comments (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
45-217: Color constants and wrappers are well-structured and toggle-awareThe separation between raw ANSI codes, toggle-aware getters, and semantic/wrapper methods (including bright/bold/underline helpers) is clean. Returning empty strings when
enabledisfalsekeeps call sites simple and prevents stray reset codes from leaking into logs/console output. This looks good.
…ble and property - Introduced `MEMBRANE_DISABLE_TERM_COLORS` environment variable and `membrane.disable.term.colors` system property. - Updated `TerminalColors`, `Constants`, and log4j2 configuration files to support new variables. - Adjusted ANSI support detection logic to accommodate the disable feature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
distribution/router/conf/log4j2.xml (1)
10-12: Consider Log4j2 and application ANSI detection alignment.The Log4j2 configuration uses a simple environment variable toggle (
${env:MEMBRANE_DISABLE_TERM_COLORS:-false}), whileTerminalColors.detectAnsiSupport()implements sophisticated detection including CI environments, IDE detection, OS checks, and terminal type detection. This means:
- Log4j2-generated logs will use ANSI colors by default unless the environment variable is explicitly set
- Application code using
TerminalColorsmay disable colors automatically based on context (CI, dumb terminal, etc.)- This could lead to inconsistent behavior where Log4j2 outputs colors but application logs don't (or vice versa)
Consider:
- Setting
MEMBRANE_DISABLE_TERM_COLORSprogrammatically in the application startup based onTerminalColors.detectAnsiSupport()before Log4j2 initialization- Or document that users in non-color-supporting environments should set the environment variable manually
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/Constants.java(2 hunks)core/src/main/java/com/predic8/membrane/core/Router.java(4 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)core/src/test/resources/log4j2.xml(1 hunks)distribution/conf/log4j2.xml(1 hunks)distribution/router/conf/log4j2.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/conf/log4j2.xml
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/Router.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-99)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (3)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-99)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java (1)
OSUtil(23-95)core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java (1)
StringUtil(21-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (12)
core/src/main/java/com/predic8/membrane/core/Router.java (3)
55-55: LGTM! Import organization is appropriate.The wildcard imports for utility packages and static imports for color constants are reasonable and improve code readability.
Also applies to: 71-71, 74-75
360-365: LGTM! Startup logging with terminal colors is well-structured.The new
logStartupMessage()helper appropriately centralizes the startup log and correctly applies colors that gracefully degrade when disabled. The conditional invocation logic properly handles both synchronous and asynchronous initialization paths.Also applies to: 680-680
686-688: LGTM! Pattern matching improves readability.The use of pattern matching for
instanceofeliminates the need for an explicit cast and makes the code more concise.distribution/router/conf/log4j2.xml (1)
5-9: LGTM! Clear documentation for disabling colors.The documentation comment provides clear instructions and a concrete example for users who need to disable terminal colors.
core/src/test/resources/log4j2.xml (2)
2-2: LGTM! Test logging configuration updated consistently.The test configuration appropriately mirrors the production logging setup with colored output support and the same environment-variable-based toggle. The addition of
status="WARN"helps reduce Log4j2 internal logging noise during tests.Also applies to: 10-12
15-22: LGTM! Logger configuration is appropriate for tests.The logger hierarchy is properly configured with the core package logger and root logger both directing output to STDOUT, which is appropriate for test environments.
core/src/main/java/com/predic8/membrane/core/Constants.java (2)
27-30: LGTM! Constants are well-organized.The new constants for terminal color control are appropriately named and organized with a helpful comment distinguishing environment variables from system properties.
49-49: LGTM! Idiomatic catch parameter naming.Renaming the catch parameter to
ignoredclearly signals the exception is intentionally not handled, which is appropriate for this fallback scenario.core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (4)
22-43: LGTM! Utility class structure is well-designed.The class correctly uses:
finalto prevent subclassingvolatilefor thread-safe flag access- Private constructor to prevent instantiation
- Static initialization for one-time ANSI support detection
45-146: LGTM! ANSI code constants and getters are correctly implemented.The private constants store standard ANSI escape sequences, and the public getter methods appropriately return empty strings when colors are disabled, providing graceful degradation. The naming convention (underscore suffix for private constants) is clear and consistent.
148-217: LGTM! Semantic aliases and wrapper methods enhance usability.The semantic aliases (ERROR, WARN, INFO, SUCCESS) and convenience wrapper methods follow a consistent pattern and correctly apply RESET() to prevent color bleeding. These provide a user-friendly API for common use cases.
222-228: User override logic is correct but double-check the comments.The logic correctly handles the "DISABLE" semantics where
yes(override)means "disable colors" (return false). The inline comments stating "Logic is reverse here!" are helpful reminders about this inverted logic. The pattern is consistent for both environment variable and system property checks.
…lColors` - Simplified terminal color detection by inlining `isDumbTerminal` logic directly into `TerminalColors`. - Removed unused method from `OSUtil`.
- Implemented detailed environment-based terminal color detection in shell and batch scripts. - Deprecated redundant methods in `OSUtil` while streamlining color detection logic in `TerminalColors`. - Updated log configuration files to support standardized `membrane.disable.term.colors` system property. - Improved user control over terminal colors with clear case-insensitive environment overrides.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/Constants.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/OSUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)core/src/test/resources/log4j2.xml(1 hunks)distribution/conf/log4j2.xml(1 hunks)distribution/router/conf/log4j2.xml(1 hunks)distribution/scripts/start_router.cmd(1 hunks)distribution/scripts/start_router.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/util/OSUtil.java
- distribution/conf/log4j2.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-02T18:05:47.059Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1947
File: distribution/tutorials/soap/membrane.sh:17-22
Timestamp: 2025-07-02T18:05:47.059Z
Learning: When using environment variable shortcuts like `$MEMBRANE_HOME` in shell scripts that look for installation directories, validate that required files (like `starter.jar` for Membrane) exist in the candidate directory before returning it. This prevents hard-to-diagnose runtime errors when the environment variable points to an invalid or stale location.
Applied to files:
core/src/main/java/com/predic8/membrane/core/Constants.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/test/resources/log4j2.xml (1)
2-13: Test logging color configuration looks coherent and matches runtime wiring
status="WARN"plus the UTF‑8PatternLayoutusingdisableAnsi="${sys:membrane.disable.term.colors:-false}"and%highlightis consistent with the new MEMBRANE_DISABLE_TERM_COLORS wiring in the rest of the codebase; no functional issues from this change.core/src/main/java/com/predic8/membrane/core/Constants.java (1)
28-37: Centralizing the color-disabling property inConstantslooks goodThe
MEMBRANE_DISABLE_TERM_COLORS_PROPERTYconstant and its JavaDoc nicely document the contract with the start scripts and log configuration, and the small catch-variable rename toignoredis a clean readability improvement.Also applies to: 53-53
distribution/router/conf/log4j2.xml (1)
5-13: Runtime logging color toggle is wired correctly to the new system propertyThe console
PatternLayout’sdisableAnsi="${sys:membrane.disable.term.colors:-false}"and the MEMBRANE_DISABLE_TERM_COLORS documentation align with the new property inConstantsand the Unix start script; once the Windows script also forwards-Dmembrane.disable.term.colors, this config will behave consistently across platforms.distribution/scripts/start_router.sh (1)
32-87: Shell color detection and JVM property wiring look solid
detect_colorscovers the documented MEMBRANE_DISABLE_TERM_COLORS override and sensible auto‑detection, and the resultingDISABLE_COLORSis correctly forwarded via-Dmembrane.disable.term.colors=$DISABLE_COLORSso TerminalColors and log4j2 can honor it; no issues from this change.core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
1-223: Property‑driven TerminalColors implementation is consistent and well‑scopedThe static,
volatile-guardedenabledflag, ANSI code constants, semantic helpers, anddetectAnsiSupport()logic (inverting themembrane.disable.term.colorssystem property and defaulting to disabled when unset) all fit together cleanly and align with the updated start scripts and log4j2 configuration; I don’t see further changes needed here.
- Extended `start_router.cmd` to include `JAVA_OPTS` for disabling terminal colors.
- Added support for WezTerm, Alacritty, and Kitty in `start_router.sh`. - Minor formatting adjustment in `TerminalColors`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
19-19: Narrow or remove the@SuppressWarnings("unused")annotation.Suppressing "unused" warnings at the class level is too broad and can hide legitimate issues. If specific methods are genuinely unused but intentionally provided as part of the public API, mark those methods individually. Otherwise, remove unused methods or the annotation entirely.
20-25: Consider adding JavaDoc to document the class contract.While the implementation is clear, adding JavaDoc would help users understand the enable/disable mechanism, the role of the system property, and the delegation to start scripts for terminal detection. This improves maintainability and reduces cognitive load for future developers.
Example JavaDoc:
/** * Provides ANSI terminal color codes with runtime enable/disable support. * <p> * Color output is controlled by the {@code membrane.disable.term.colors} system property, * typically set by the start script based on terminal capability detection. * When the property is not set, colors are disabled by default. * <p> * All color code getters return empty strings when colors are disabled, allowing * safe concatenation without conditional logic. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)distribution/scripts/start_router.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/scripts/start_router.sh
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (4)
22-22: Thread-safety approach is sound.The
volatilemodifier ensures visibility of theenabledfield across threads, which is appropriate for this toggle pattern. The simple read/write operations don't require stronger synchronization.Also applies to: 27-37
39-140: Clean API design with appropriate encapsulation.The pattern of private ANSI code constants with public getters that return empty strings when disabled is well-designed. This allows callers to concatenate color codes without null checks or conditional logic, and the enable/disable toggle is cleanly enforced at the access layer.
142-211: Semantic aliases and convenience wrappers improve usability.The semantic color names (ERROR, WARN, INFO, SUCCESS) and wrapping methods that automatically apply RESET() make the API more intuitive and reduce boilerplate for callers. The implementation correctly handles the enabled/disabled state through the underlying getter methods.
213-222: Detection logic is appropriate for script-driven configuration.The implementation correctly delegates terminal capability detection to the start script (as documented in Constants.java), reading only the system property. The conservative default of disabling colors when the property is absent is sensible—it indicates the start script hasn't run or hasn't set the property. The inversion on line 218 correctly handles the "disable" semantics of the property name.
- Added support for WezTerm, Alacritty, and Kitty in `start_router.sh`. - Minor formatting adjustment in `TerminalColors`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (2)
29-29: Consider narrowing the scope of @SuppressWarnings.The class-level
@SuppressWarnings("unused")suppresses warnings for all methods, which may hide legitimate unused code. Consider removing it once all public methods are in use, or applying it only to specific methods that are intentionally part of the public API but not yet utilized.
169-221: Document or warn about nesting behavior of wrapper methods.Each wrapper method automatically appends
RESET(), which will cancel any outer styling if these methods are nested. For example,bold(error("text"))producesBOLD + ERROR + "text" + RESET + RESET, where the innerRESET()cancels the bold styling, leaving only plain "text" visible.Consider adding a JavaDoc note on these methods warning that they should not be nested, or provide alternative methods that don't auto-reset for composition scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/main/java/com/predic8/membrane/core/util/text/TerminalColors.java (1)
223-232: LGTM! Conservative and well-documented detection logic.The detection method correctly reads the system property, inverts the "disable" semantics, and defaults to disabled when the property is not set. This conservative default prevents unexpected ANSI codes in environments where terminal support is uncertain.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.